newton/_src/utils/import_usd.py (1)

527-528: Defer shape_color resolution until a supported geometry type is known.

Line 528 now resolves material bindings for every uncached child, including Xform and other nodes that never create a shape. Moving that lookup behind the supported-geometry branches avoids extra traversal work and cache entries on large USD hierarchies.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/utils/import_usd.py` around lines 527 - 528, The code currently
calls _get_material_props_cached(prim) for every uncached child (path_name) even
for non-geometry prims; move the shape_color lookup so it only runs after you
detect a supported geometry type for prim (e.g., inside the branches that handle
Mesh/Sphere/Cube or after checking prim.GetTypeName()/UsdGeom predicates),
leaving the existing path_name not in path_shape_map check in place but
deferring shape_color resolution until inside the geometry-specific branches
where a shape is actually created; use the same variables (path_name, prim,
shape_color, _get_material_props_cached, path_shape_map) to locate and update
the logic.

newton/tests/test_sensor_tiled_camera.py (1)

12-12: Keep the test oracle independent of Newton’s own converter.

Line 12 imports linear_to_srgb_rgb(), and Lines 208-212 use it to derive the expected bytes. That makes the assertion less independent of the production color-space code. A tiny local reference conversion or fixed expected bytes would make this regression test stronger.

Also applies to: 208-212

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/tests/test_sensor_tiled_camera.py` at line 12, The test imports and
uses Newton’s linear_to_srgb_rgb() converter to build the expected byte results,
making the oracle dependent on production code; remove the import of
linear_to_srgb_rgb from test_sensor_tiled_camera.py and replace the
derived-expected logic (the code around where expected bytes are computed) with
either a small, self-contained local conversion implementation (e.g., the sRGB
gamma mapping applied to linear RGB values used in the test) or with fixed
hard-coded expected byte values; update the assertion to use that local
converter or fixed bytes and ensure you do not call linear_to_srgb_rgb() from
the production module in the test.

newton/_src/sensors/sensor_tiled_camera.py (1)

210-213: Clarify where encode_output_srgb is configured in the update() docs.

update() does not take a render_config argument, so render_config.encode_output_srgb=False is ambiguous here. Consider referencing self.render_config.encode_output_srgb (or setting it at construction via config=SensorTiledCamera.RenderConfig(...)).

✏️ Suggested doc wording tweak

-            color_image: Output for packed RGBA color. The bytes are sRGB by
-                default, or linear when
-                ``render_config.encode_output_srgb=False``. None to skip.
+            color_image: Output for packed RGBA color. The bytes are sRGB by
+                default, or linear when
+                ``self.render_config.encode_output_srgb=False``. None to skip.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/sensors/sensor_tiled_camera.py` around lines 210 - 213, Docstring
refers to render_config.encode_output_srgb ambiguously; update the docs to point
to the instance config by referencing self.render_config.encode_output_srgb (or
note that it can be set at construction via SensorTiledCamera.RenderConfig
passed as config) and clarify that update() itself does not accept a
render_config argument—update the parameter descriptions for
color_image/depth_image accordingly.

newton/tests/test_import_usd.py (1)

20-20: Keep these sRGB expectations independent from production code.

Importing srgb_to_linear_rgb from newton._src.utils.color means these assertions can still pass if the shared conversion logic regresses in the same way on both sides. Prefer fixed reference values, or at least a dedicated test-local reference implementation, for these expectations.

Also applies to: 6537-6556, 6558-6585, 6587-6617, 6620-6651

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/tests/test_import_usd.py` at line 20, The test imports the production
function srgb_to_linear_rgb which makes the test pass even if both test and
production code regress; remove the import of
newton._src.utils.color.srgb_to_linear_rgb and replace usage with either
hard-coded reference output values (fixed decimals) or a small test-local
implementation of the sRGB→linear conversion inside
newton/tests/test_import_usd.py, and update the assertions to compare against
those fixed values; apply the same change to the other related test blocks
referenced (around the ranges 6537-6556, 6558-6585, 6587-6617, 6620-6651) so all
sRGB expectations are independent of production code.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

newton/_src/geometry/types.py (1)

877-879: Normalize texture_color_space in the setter to prevent texture-cache fragmentation.

Storing raw values allows equivalent metadata tokens to diverge (None/"auto"/vendor aliases), which can duplicate texture entries downstream.

♻️ Proposed fix

-from ..utils.texture import compute_texture_hash
+from ..utils.color import normalize_texture_color_space
+from ..utils.texture import compute_texture_hash
@@
     `@texture_color_space.setter`
     def texture_color_space(self, value: str):
-        self._texture_color_space = value
+        self._texture_color_space = normalize_texture_color_space(value)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/geometry/types.py` around lines 877 - 879, The
texture_color_space setter currently stores raw inputs which fragments the
texture cache; update the texture_color_space setter to normalize inputs before
assigning to self._texture_color_space: coerce None to a canonical token (e.g.
"auto"), trim/normalize case, and map known vendor aliases or synonyms (e.g.
"srgb", "sRGB", vendor-specific names) to a small set of canonical tokens;
implement this normalization inside the texture_color_space property setter so
callers use the normalized value consistently.

newton/tests/test_import_usd.py (1)

6695-6723: Make the "auto" expectation explicit in the mock.

This assertion is currently passing even if resolve_material_properties_for_prim() regresses, because the mock never returns a texture_color_space. If this test is meant to pin the resolved default, include "texture_color_space": "auto" in the mocked payload instead of relying on downstream fallback.

Suggested change

             mock.patch(
                 "newton._src.utils.import_usd.usd.resolve_material_properties_for_prim",
                 return_value={
                     "color": None,
                     "roughness": 0.35,
                     "metallic": 0.75,
                     "texture": "dummy.png",
+                    "texture_color_space": "auto",
                 },
             ),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/tests/test_import_usd.py` around lines 6695 - 6723, The mock for
resolve_material_properties_for_prim used in the test should explicitly include
the texture_color_space key to assert the resolved default; update the
return_value in the mock patch for
"newton._src.utils.import_usd.usd.resolve_material_properties_for_prim" to
include "texture_color_space": "auto" so that the assertion comparing
builder.shape_source[collision_shape].texture_color_space to "auto" is driven by
the mocked payload (affecting the builder.add_usd flow and the shape source
created by newton.ModelBuilder).

newton/_src/utils/color.py (1)

26-80: Consider upgrading function docstrings to full Google style for user-facing helpers.

These docstrings are clear, but adding Args:/Returns: blocks would align with repository standards for parameterized APIs.

As per coding guidelines, "Follow Google-style docstrings. Types in annotations, not docstrings. Use Args: with name: description format."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/utils/color.py` around lines 26 - 80, Several public helper
functions (srgb_to_linear_rgb, linear_to_srgb_rgb, linear_rgb_to_srgb_uint8,
linear_image_to_srgb_uint8, normalize_texture_color_space) use short one-line
docstrings; update each to Google-style docstrings that include an "Args:"
section (parameter name: brief description, do NOT duplicate type info since
types are in annotations) and a "Returns:" section describing the returned
value/shape/units, and any raised exceptions (e.g., ValueError from
linear_image_to_srgb_uint8). Keep descriptions concise and user-facing, follow
existing wording but expand into the Args/Returns format to match repo
guidelines.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

newton/_src/sim/builder.py (1)

196-199: ⚠️ Potential issue | 🟡 Minor

Return a clear error for malformed default_shape_color.

default_shape_color is public state, but malformed values still flow into _coerce_shape_color() and can raise IndexError/low-level conversion errors while adding a shape. Please wrap coercion failures in a descriptive ValueError.

Suggested fix

 def _coerce_shape_color(color: Vec3 | None) -> tuple[float, float, float] | None:
     if color is None:
         return None
-    return (float(color[0]), float(color[1]), float(color[2]))
+    try:
+        return (float(color[0]), float(color[1]), float(color[2]))
+    except (IndexError, TypeError, ValueError) as e:
+        raise ValueError("Shape colors must provide three numeric RGB components.") from e

Also applies to: 797-800, 5298-5299

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/sim/builder.py` around lines 196 - 199, The _coerce_shape_color
function should catch malformed inputs and raise a clear ValueError instead of
letting IndexError/TypeError/ValueError propagate; modify _coerce_shape_color to
validate and/or wrap the tuple conversion in a try/except that catches
IndexError, TypeError and ValueError and re-raises a descriptive ValueError that
includes the invalid default_shape_color value and the original exception
message. Apply the same defensive wrapping to any other spots where the same
coercion logic is implemented or called (the other occurrences noted in the
review) so callers receive a consistent, user-friendly error.

newton/_src/usd/utils.py (1)

1120-1135: ⚠️ Potential issue | 🟠 Major

mesh_out._texture_color_space is still not populated from material_props.

resolve_material_properties_for_prim() now returns "texture_color_space", but Mesh(...) here doesn't receive it (no constructor arg) and the attribute isn't assigned post-construction. Downstream consumers — notably the viewer's texture_mode branch in newton/_src/viewer/viewer.py L1580-1582 — rely on Mesh.texture_color_space == "raw" to avoid the sRGB→linear shader path, so the raw branch will never fire for USD-imported meshes until this is wired through.

💡 Suggested fix

     mesh_out = Mesh(
         points,
         faces.flatten(),
         normals=normals,
         uvs=uvs,
         maxhullvert=maxhullvert,
         color=material_props.get("color"),
         texture=material_props.get("texture"),
         metallic=material_props.get("metallic"),
         roughness=material_props.get("roughness"),
     )
+    if mesh_out.texture is not None:
+        mesh_out._texture_color_space = material_props.get(
+            "texture_color_space", TEXTURE_COLOR_SPACE_AUTO
+        )

If Mesh.__init__ accepts texture_color_space as a keyword, prefer passing it there instead of touching the private attribute.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/usd/utils.py` around lines 1120 - 1135, The Mesh created from
resolve_material_properties_for_prim(prim) is not getting the returned
"texture_color_space" so mesh_out.texture_color_space remains unset; update the
Mesh construction in utils.py to accept and set texture_color_space from
material_props (prefer passing
texture_color_space=material_props.get("texture_color_space") into the Mesh(...)
call if Mesh.__init__ supports it, otherwise assign mesh_out.texture_color_space
= material_props.get("texture_color_space") immediately after construction) so
downstream logic (e.g. the viewer's texture_mode branch that checks
Mesh.texture_color_space == "raw") behaves correctly.

newton/_src/viewer/viewer_viser.py (1)

575-579: LGTM — conversion aligns with the new color-space convention.

Using srgb_image_to_uint8 here correctly clips to [0, 1], rounds, and casts to uint8, replacing ad-hoc *255 scaling. Input shape (N, 3) satisfies the helper's contract.

Minor observation (out of scope for this change): log_points (Lines 960-970) and the _rgb_to_uint8_array helper in log_lines (Lines 832-837) still perform manual float→uint8 scaling and don't go through the shared helper. Worth unifying in a follow-up for consistency with the PR's color-space rules.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/viewer/viewer_viser.py` around lines 575 - 579, Update the manual
float→uint8 conversions to use the shared helper srgb_image_to_uint8: replace
the ad-hoc scaling in the log_points function and the _rgb_to_uint8_array helper
used by log_lines with calls to srgb_image_to_uint8 so clipping/rounding/casting
follow the new color-space convention; ensure inputs are shaped/reshaped to
(N,3) as required and preserve existing return types and caching behavior in
log_lines and log_points.

newton/_src/geometry/types.py (2)

183-186: Stray string literal after assignment — not a real docstring.

The triple-quoted string on Line 186 follows a plain attribute assignment and is not attached to anything; Sphinx/help will never see it. The authoritative docstring already lives on the texture_color_space property (Lines 932-937), so this adds nothing and can be misleading when grepping the class for attribute docs.

🧹 Optional cleanup

         self._texture = _normalize_texture_input(texture)
-        self.texture_color_space: str = "auto"
-        """Source color space of :attr:`texture`: ``"auto"``, ``"srgb"``, or ``"raw"``."""
+        # Initializes self._texture_color_space via the property setter below.
+        self.texture_color_space = "auto"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/geometry/types.py` around lines 183 - 186, Remove the stray
triple-quoted string literal that sits immediately after the attribute
assignment to self.texture_color_space in the class initializer; it is not a
real docstring and should be deleted so it doesn't clutter searches or confuse
readers—locate the block where self._texture is set and texture_color_space: str
= "auto" is assigned and remove the following standalone triple-quoted string
(the genuine docstring lives on the texture_color_space property further down).

---

930-942: Consider validating texture_color_space values.

The property docstring narrows the allowed values to {"auto", "srgb", "raw"}, and downstream consumers branch on these literals (raytracer dedup key / sRGB-vs-raw sampling). The setter currently accepts any string, so typos like "sRGB" or "linear" will silently fall through as if they were "auto" in some code paths and as unknown in others. A simple allow-list check in the setter would make misuse fail fast.

🛡️ Suggested validation

     `@texture_color_space.setter`
     def texture_color_space(self, value: str):
+        allowed = ("auto", "srgb", "raw")
+        if value not in allowed:
+            raise ValueError(
+                f"texture_color_space must be one of {allowed}, got {value!r}"
+            )
         self._texture_color_space = value

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/geometry/types.py` around lines 930 - 942, The
texture_color_space setter currently accepts any string; add validation in the
texture_color_space setter to only allow the literals "auto", "srgb", and "raw"
(optionally normalizing input to lowercase first), and raise a ValueError with a
clear message if an invalid value is passed; update the internal field
self._texture_color_space only after validation so consumers that branch on
texture_color_space see only the allowed values (refer to the
texture_color_space property/setter and the _texture_color_space attribute).

newton/_src/viewer/viewer.py (1)

1588-1588: Reaching into ModelBuilder._DEFAULT_GROUND_PLANE_COLOR couples the viewer to a private constant.

Consider exposing the default ground-plane color through a public surface (e.g., a ModelBuilder.DEFAULT_GROUND_PLANE_COLOR classvar, a module-level constant in newton/_src/sim/builder.py, or a helper on the builder) so the viewer doesn't depend on a leading-underscore symbol. The equivalent default is already duplicated at line 820 as wp.vec3(0.125, 0.125, 0.25) (note the different third component) — unifying these two constants avoids them drifting apart further.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/viewer/viewer.py` at line 1588, The viewer is reading the private
constant ModelBuilder._DEFAULT_GROUND_PLANE_COLOR which couples viewer.py to a
leading-underscore symbol and duplicates a similar constant elsewhere; change
the builder to expose a public constant (e.g., add
ModelBuilder.DEFAULT_GROUND_PLANE_COLOR or export a module-level
DEFAULT_GROUND_PLANE_COLOR in newton/_src/sim/builder.py) and update viewer.py
to use that public symbol instead of ModelBuilder._DEFAULT_GROUND_PLANE_COLOR,
and also consolidate the duplicated value (the wp.vec3(0.125, 0.125, 0.25)
definition) so both locations reference the single public constant.

newton/_src/usd/utils.py (1)

1607-1617: Dead conditional and an unexplained 0.18 fudge factor in _multiply_colors.

Two things:

1. Line 1615 ... if tint is not None else None is unreachable — the early return on line 1612 already guarantees tint is not None at this point.
2. 0.18 is OmniPBR's implicit middle-grey base assumption when only a tint is authored. Without a comment this reads as a magic number; it also silently fabricates a color that will then block the displayColor fallback in _resolve_prim_material_properties (line 1948 checks properties["color"] is None). Worth deciding whether tint-only materials should synthesize a mid-grey base or defer to displayColor — currently they do the former and skip the latter.

♻️ Suggested cleanup

-def _multiply_colors(
-    color: tuple[float, float, float] | None,
-    tint: tuple[float, float, float] | None,
-) -> tuple[float, float, float] | None:
-    """Multiply two RGB colors componentwise."""
-    if tint is None:
-        return color
-    if color is None:
-        return tuple(t * 0.18 for t in tint) if tint is not None else None
-    return tuple(float(c * t) for c, t in zip(color, tint, strict=True))
+# OmniPBR assumes an 18% middle-grey diffuse reflectance when no base color
+# is authored but a diffuse tint is present.
+_OMNI_PBR_DEFAULT_DIFFUSE = 0.18
+
+def _multiply_colors(
+    color: tuple[float, float, float] | None,
+    tint: tuple[float, float, float] | None,
+) -> tuple[float, float, float] | None:
+    """Multiply two RGB colors componentwise.
+
+    If only ``tint`` is authored, treat the base as OmniPBR's 18% middle grey.
+    """
+    if tint is None:
+        return color
+    if color is None:
+        return tuple(float(t * _OMNI_PBR_DEFAULT_DIFFUSE) for t in tint)
+    return tuple(float(c * t) for c, t in zip(color, tint, strict=True))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/usd/utils.py` around lines 1607 - 1617, The helper
_multiply_colors has an unreachable conditional and a magic 0.18 factor; update
the function to (a) remove the dead `if tint is not None else None` branch and
simplify the early returns, and (b) choose one behavior for tint-only inputs:
either synthesize a documented mid-grey by replacing the literal 0.18 with a
named constant (e.g. MID_GREY_FACTOR) and add a comment explaining OmniPBR
middle-grey semantics, or return None so that
`_resolve_prim_material_properties` can fall back to `displayColor`; also
mention `_resolve_prim_material_properties` in the comment so future readers
know why the choice matters.

newton/tests/test_sensor_tiled_camera.py (1)

12-12: Use the public color utility import.

This test is validating public tiled-camera color behavior, so it should avoid binding to the private _src layout.

Suggested change

-from newton._src.utils.color import srgb_to_linear_rgb
+from newton.utils import srgb_to_linear_rgb

As per coding guidelines, “Do not import from newton._src in examples and docs. Expose user-facing symbols via public modules (newton/geometry.py, newton/solvers.py, etc.).”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/tests/test_sensor_tiled_camera.py` at line 12, The test imports
srgb_to_linear_rgb from the private path newton._src.utils.color; update the
import to use the public API instead by importing srgb_to_linear_rgb from the
top-level public module that re-exports it (e.g., from newton.color or another
public module that exposes srgb_to_linear_rgb), so change the import of
srgb_to_linear_rgb to use the public symbol and remove any direct references to
newton._src in test_sensor_tiled_camera.py.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes